Conversation
ZhongpinWang
left a comment
ZhongpinWang
left a comment
There was a problem hiding this comment.
Some clean code ideas. Bear my first Java PR review with some old JS habits. 😁
| "gzip".equals(headerParams.get("Content-Encoding")) | ||
| ? serializeGzip(body, contentTypeObj) | ||
| : serialize(body, formParams, contentTypeObj)); |
There was a problem hiding this comment.
[pp] Maybe this part can be extracted into a separate function and do "switch case" for different encodings? Just for better code structure since gzip is only one of the many possible values of content encoding.
...core-apache/src/main/java/com/sap/cloud/sdk/services/openapi/apache/apiclient/ApiClient.java
Outdated
Show resolved
Hide resolved
| } | ||
| } | ||
|
|
||
| private HttpEntity serializeGzip( final Object body, final ContentType contentType ) |
There was a problem hiding this comment.
[pp/req] (req for request changes, but up to you to decide) Also the current serializeGzip() cannot handle form data?
What about using the current serialize() method first, after getting the HttpEntity, check the Content-Encoding and call HttpEntity::writeTo() to write the content to OutputStream and then compress? Makes code much cleaner IMO.
The ApiClient is straight from OpenAPI Generator. We shouldn't put any effort towards generated code or modified code from OpenAPI. |
I now get your point. But aren't you then editing a generated file? How would you make |
Jonas-Isr
left a comment
Jonas-Isr
left a comment
There was a problem hiding this comment.
I still feel like we should add some minimal (vibe-coded?) unit tests. I don't think the argument that this is tested through AI SDK is strong enough: If we ever discontinue the feature in AI SDK this will not be tested anymore and potentially break without us noticing.
...core-apache/src/main/java/com/sap/cloud/sdk/services/openapi/apache/apiclient/ApiClient.java
Outdated
Show resolved
Hide resolved
3 participants
Context
SAP/cloud-sdk-java-backlog#363.
Smaller payload.
Corresponding AI SDK PR:
Feature scope:
Content-Encodingis set togzip-> compress the request payloadDefinition of Done
Error handling created / updated & covered by the tests aboveDocumentation updatedRelease notes updated